Add operator APIs for manipulating system IP Pools #9225
Conversation
bnaecker
force-pushed
the
add-system-ip-pool-apis
branch
from
4a65598 to
eed8606
Compare
| } | ||
|
|
||
| async fn ip_pool_range_list( | ||
| async fn system_ip_pool_range_list( |
There was a problem hiding this comment.
Note to self -- I think this should be a "normal", non-system endpoint. Users who can see an IP Pool should be able to list its ranges. But it does make the whole prefix / no-prefix thing more confusing.
There was a problem hiding this comment.
I talked a while ago with @ahl about this. I think we actually want to have a system and non-system endpoint for listing and viewing pools, and for listing ranges in a pool. The non-system versions are basically the same as the existing "project-scoped" implementations, showing the resources available for use in the silo of the calling user.
- Add Nexus endpoints for reserving IP Pools for Oxide use or for external customer Silos - Remove check used to hide internal IP Pools, since they're all visible to an operator now - Rename operator IP Pool endpoints by prefixing `system_*`, to avoid conflicts with previous "silo-scoped" endpoints. - Handle possibility of any number of Oxide-internal IP Pools in various places where we previously assumed exactly 2 of them. - Fixes #8947 - Fixes #8226
bnaecker
force-pushed
the
add-system-ip-pool-apis
branch
from
eed8606 to
143f98a
Compare
bnaecker
requested review from
ahl,
david-crespo,
jgallagher and
rcgoodfellow
|
We should probably hold this PR until after we cut the R17 release. I'm also adding a PR to the CLI repo handling these breaking changes. |
rcgoodfellow
left a comment
rcgoodfellow
left a comment
There was a problem hiding this comment.
Thanks @bnaecker! Comments follow.
| } | ||
|
|
||
| // List Oxide reserved IP Pools and corresponding authz objects. | ||
| async fn list_all_service_ip_pools( |
There was a problem hiding this comment.
This function is not really doing what the name/comment suggests. It's associating particular IP pools with operator specified RSS ranges.
There was a problem hiding this comment.
Fair point, I'll change it. I think it's only call-site is when doing the initial deployment after rack setup, which means the whole function should go away when we supply the pools through RSS (rather than just the ranges, which we do today).
| /// The pool is reserved for use by external customer Silos. | ||
| ExternalSilos, | ||
| /// The pool is reserved for Oxide internal use. | ||
| OxideInternal, |
There was a problem hiding this comment.
Recommend using SystemInternal for outward facing stuff to be consistent with system scoped APIs.
There was a problem hiding this comment.
Yeah, that's better. I'll rename this. It will make this PR a bit larger though. I already named it like that in the database, so I'll see if I can easily rename the variant there. That can be tricky IIRC, so it might not work.
| .ip_pool_list_ranges_batched(opctx, &authz_pool) | ||
| .await | ||
| .internal_context("listing services pool ranges")?; | ||
| all_ranges.append(&mut ranges); |
There was a problem hiding this comment.
Just making sure I understand: this is collapsing "all the ranges from all the pools" into just a list of "all the ranges" for Reconfigurator, right? It will still think there are really just two pools (one v4, one v6).
I think this all fine as long as the pools don't have any intent behind them ("this pool is for {Nexus,NTP,DNS}").
There was a problem hiding this comment.
Yes, this collapses all the ranges from all the pools. With this PR, it's possible to have more than 2 pools delegated to Oxide, but they don't have any particular meaning yet beyond their name. I imagine that to be a part of #9313 or one of its child issues.
system_*, to avoid conflicts with previous "silo-scoped" endpoints.project_ip_pool_listandproject_ip_pool_view#8226